fix: validate profile constructor inputs (#440 — B9, B11, B12) + B10 tolerance pin - #566
Merged
Merged
Conversation
…tolerance pin Four findings from @rhayes777's API audit, all still reproducing on main. - B9 scale_radius of 0.0 / negative / nan -> guarded at the 5 assignment sites covering the whole halo family (AbstractgNFW, cNFW x2, Kaplinghat, Yang24). A zero scale radius divided the grid by zero and returned all-NaN deflections (3200 of 3200) rather than raising — the profile appeared to work. - B11 sersic_index of 0.0 / negative / nan -> guarded at both Sersic bases (the light profile and the stellar mass profile). Previously a bare ZeroDivisionError from inside image_2d_from. - B12 ell_comps outside the unit circle -> guarded once at EllProfile, the single base every elliptical light and mass profile inherits. q = (1-f)/(1+f) is only a valid axis ratio for f < 1; beyond it q goes negative and the profile has no geometric meaning, yet it returned a finite non-physical image. - B10 is a tolerance pin, not a fix: Isothermal(ell_comps=(0,0)) vs IsothermalSph are analytically identical, numerically not. Pinned so a future refactor that makes the agreement materially worse is caught. Guards delegate to the shared autoarray.validate helpers established for #333, so all three repos give one message for one class of mistake. Per-parameter explanations live once in autogalaxy/profiles/validate.py rather than per class. Tracer-safe: gated on autoarray.validate.is_concrete_scalar, so a traced free model parameter passes through untouched and no Python truth-test reaches a tracer. NOTE for review: while pinning B10 the potential was measured as well as the deflections the reporter reported. The potential agrees only to 1.9e-03 in relative terms, three orders of magnitude worse than deflections (2.4e-06) and convergence (1.5e-06). That is NOT part of B10 as filed and is not fixed here; its tolerance is pinned at the observed level as a ratchet, and flagged for its own investigation. Tests: 30 new cases in test_autogalaxy/profiles/test_validate.py, one per finding from the reporter's snippets plus a control per finding. Suite 1074 passed, zero regressions. Closes #440. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013PgqSCLTemK5bApVAwhVM4
Jammy2211
added
the
pending-release
Claude
…s here) The negative-redshift finding is filed on PyAutoLens#532 because the reporter reached it through `al.Galaxy` — but `al.Galaxy` IS `ag.Galaxy`, and both the class and its redshift assignment live in this repo, so the guard belongs here rather than in a Tracer-level check that would miss a bare Galaxy construction. The `Tracer(galaxies=...)` half of #532 stays in PyAutoLens. Zero and tiny redshifts stay accepted: 0.0 legitimately places a galaxy at the observer, and 1e-12 is degenerate but not invalid. Adds a deliberate guard-rail test pinning today's permissive z_lens > z_source behaviour, so phase 4 of the audit cannot quietly turn it into an error while the question is still open with the reporter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013PgqSCLTemK5bApVAwhVM4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #440. Phase 2 (PyAutoGalaxy half) of the @rhayes777 API audit epic PyAutoArray#415, which stays open for phases 3-4.
Follows PyAutoArray#440 (merged
f2f7a4f), which established the shared helper this PR imports.What this fixes
mp.NFW(scale_radius=0.0)ValueErrornamingscale_radiuslp.Sersic(sersic_index=0.0)ZeroDivisionErrorfrom insideimage_2d_fromValueErrornamingsersic_indexlp.Sersic(ell_comps=(2.0, 0.0))ValueErrornamingell_compsand its magnitudemp.Isothermal(ell_comps=(0,0))vsmp.IsothermalSphGuards sit at shared bases, so coverage is wider than the report:
EllProfile, the single base every elliptical light and mass profile inherits.ell_compshas exactly one assignment site in the codebase, so the rule is stated once rather than per subclass, as the issue asked.scale_radiusassignment sites, covering the whole halo family (AbstractgNFW→ NFW/gNFW/NFWTruncated/…,cNFW×2,KaplinghatCoredNFWSph,YangSIDMSph) — 29 dark profile classes.AbstractSersicbases: the light profile and the stellar mass profile.B10 — and something the audit missed
B10 is a tolerance pin, not a fix.
Isothermal(ell_comps=(0,0))andIsothermalSphare analytically identical; numerically they differ because the elliptical form takes a different evaluation route even at the degenerate point. Bit-identity is explicitly not the goal — the pin exists so a refactor that makes agreement materially worse gets caught.While pinning it I measured the potential as well as the deflections, which the report covered:
The potential agrees three orders of magnitude worse than the other two. That is not part of B10 as filed — @rhayes777 measured deflections only — and it is not fixed here. Its tolerance is pinned at the currently-observed level as a ratchet, with a docstring saying exactly that, rather than silently folded into a loose bound that would hide it. It likely wants its own investigation; there is an adjacent open prompt on NFW-truncated potential MGE accuracy that may be the same class of problem.
Flagging it explicitly rather than burying it in a tolerance is the point.
Shared helper, not a reimplementation
Rules and message shape come from
autoarray.validate(PyAutoArray#440).autogalaxy/profiles/validate.pyholds only the per-parameter explanation, written once per parameter instead of once per profile class. Three repos, one message for one class of mistake.API Changes
No signatures, names or return types change. The change is in accepted input domain:
scale_radiusmust be finite and> 0(previously0.0, negative andnanaccepted).sersic_indexmust be finite and> 0(previously accepted, failing later).ell_compsmust satisfye_y**2 + e_x**2 < 1(previously any magnitude accepted). Note|e| == 1is also now rejected: it degenerates toq == 0.Tracer safety
Profile parameters are free model parameters, so under a traced fit a constructor receives a JAX tracer. Every guard is gated on
autoarray.validate.is_concrete_scalarand passes non-concrete values through untouched — the same form verified against real JAX in PyAutoArray#440.Test Plan
test_autogalaxy/profiles/test_validate.py— 30 cases. One regression test per finding from the reporter's snippets, plus a control per finding so no guard passes by rejecting everything, plus tracer-safety cases.1074 passed, 1 skipped. Zero regressions, and no existing test relied on a degenerate profile construction.Out of scope
Tracerguards — sibling prompt, also unblocked by the shared helper.adapt_imagesprecondition — phase 3.z_lens > z_source— phase 4, HELD awaiting @rhayes777's answer.Generated by Claude Code